Hi @wcheang. Thanks for your PR.

I'm waiting for a konflux-ci member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

If you are adding things to releaseNotes the schema needs to be updated https://github.com/konflux-ci/release-service-catalog/blob/development/schema/dataKeys.json

Thanks @johnbieren, added the schema.

My PTO is starting tomorrow, so @jedinym from my team will be taking over getting this PR merged.

If you are adding things to releaseNotes the schema needs to be updated https://github.com/konflux-ci/release-service-catalog/blob/development/schema/dataKeys.json

This is added during the release pipeline. I thought the schema only covers what's there at the start, but apparently I was wrong. What's the purpose of having releaseNotes.content.images there? It will never be validated, no?

If you are adding things to releaseNotes the schema needs to be updated https://github.com/konflux-ci/release-service-catalog/blob/development/schema/dataKeys.json

This is added during the release pipeline. I thought the schema only covers what's there at the start, but apparently I was wrong. What's the purpose of having releaseNotes.content.images there? It will never be validated, no?

The releaseNotes stuff is unique. We don't overwrite. We =+ https://github.com/konflux-ci/release-service-catalog/blob/development/tasks/populate-release-notes-images/populate-release-notes-images.yaml#L111
A user could define the same stuff the task is adding for them. I don't know why they'd want to, but they could. So, if a field could be provided by the user, I think it should be in the schema

If you are adding things to releaseNotes the schema needs to be updated https://github.com/konflux-ci/release-service-catalog/blob/development/schema/dataKeys.json

This is added during the release pipeline. I thought the schema only covers what's there at the start, but apparently I was wrong. What's the purpose of having releaseNotes.content.images there? It will never be validated, no?

The releaseNotes stuff is unique. We don't overwrite. We =+ https://github.com/konflux-ci/release-service-catalog/blob/development/tasks/populate-release-notes-images/populate-release-notes-images.yaml#L111 A user could define the same stuff the task is adding for them. I don't know why they'd want to, but they could. So, if a field could be provided by the user, I think it should be in the schema

I'm not completely sure I buy this argument - it seems to me that being theoretically able to set this already in the RPA is more of a side effect. I'm not even sure if that should be allowed at all - it should say what was actually included in that release, right? So users shouldn't really be able to say that something extra is being released too when it's not. Or am I missing something?

I replied on the ticket, but why are we adding new fields to the advisories? Shouldn't this be driven by prodsec? Maybe it already is, but I haven't seen it

Good point. I completely missed the fact that this will also add these fields to the resulting advisory json.

If you are adding things to releaseNotes the schema needs to be updated https://github.com/konflux-ci/release-service-catalog/blob/development/schema/dataKeys.json

This is added during the release pipeline. I thought the schema only covers what's there at the start, but apparently I was wrong. What's the purpose of having releaseNotes.content.images there? It will never be validated, no?

The releaseNotes stuff is unique. We don't overwrite. We =+ https://github.com/konflux-ci/release-service-catalog/blob/development/tasks/populate-release-notes-images/populate-release-notes-images.yaml#L111 A user could define the same stuff the task is adding for them. I don't know why they'd want to, but they could. So, if a field could be provided by the user, I think it should be in the schema

I'm not completely sure I buy this argument - it seems to me that being theoretically able to set this already in the RPA is more of a side effect. I'm not even sure if that should be allowed at all - it should say what was actually included in that release, right? So users shouldn't really be able to say that something extra is being released too when it's not. Or am I missing something?

I am referring to adding it to the Release data section, not ReleasePlanAdmission

If you are adding things to releaseNotes the schema needs to be updated https://github.com/konflux-ci/release-service-catalog/blob/development/schema/dataKeys.json

This is added during the release pipeline. I thought the schema only covers what's there at the start, but apparently I was wrong. What's the purpose of having releaseNotes.content.images there? It will never be validated, no?

The releaseNotes stuff is unique. We don't overwrite. We =+ https://github.com/konflux-ci/release-service-catalog/blob/development/tasks/populate-release-notes-images/populate-release-notes-images.yaml#L111 A user could define the same stuff the task is adding for them. I don't know why they'd want to, but they could. So, if a field could be provided by the user, I think it should be in the schema

I'm not completely sure I buy this argument - it seems to me that being theoretically able to set this already in the RPA is more of a side effect. I'm not even sure if that should be allowed at all - it should say what was actually included in that release, right? So users shouldn't really be able to say that something extra is being released too when it's not. Or am I missing something?

I am referring to adding it to the Release data section, not ReleasePlanAdmission

What difference does it make? I understand that if you create a manual Release then you know the snapshot and so you can enter correct data about images in the snapshot into the Release data section, but those would then be duplicated in our task anyway. So not sure what the point would be.

If you are adding things to releaseNotes the schema needs to be updated https://github.com/konflux-ci/release-service-catalog/blob/development/schema/dataKeys.json

This is added during the release pipeline. I thought the schema only covers what's there at the start, but apparently I was wrong. What's the purpose of having releaseNotes.content.images there? It will never be validated, no?

The releaseNotes stuff is unique. We don't overwrite. We =+ https://github.com/konflux-ci/release-service-catalog/blob/development/tasks/populate-release-notes-images/populate-release-notes-images.yaml#L111 A user could define the same stuff the task is adding for them. I don't know why they'd want to, but they could. So, if a field could be provided by the user, I think it should be in the schema

I'm not completely sure I buy this argument - it seems to me that being theoretically able to set this already in the RPA is more of a side effect. I'm not even sure if that should be allowed at all - it should say what was actually included in that release, right? So users shouldn't really be able to say that something extra is being released too when it's not. Or am I missing something?

I am referring to adding it to the Release data section, not ReleasePlanAdmission

What difference does it make? I understand that if you create a manual Release then you know the snapshot and so you can enter correct data about images in the snapshot into the Release data section, but those would then be duplicated in our task anyway. So not sure what the point would be.

Because you want an image in your advisory that is not part of your snapshot or isn't mapped or something. I don't know. I am just saying it is a possible use case so it should be in the schema

I replied on the ticket, but why are we adding new fields to the advisories? Shouldn't this be driven by prodsec? Maybe it already is, but I haven't seen it

@johnbieren If I understand correctly, the new fields in the advisory are not the intention here. They are only a side effect of adding additional data for the update-component-sbom task. I will refactor this PR to avoid that issue.

I replied on the ticket, but why are we adding new fields to the advisories? Shouldn't this be driven by prodsec? Maybe it already is, but I haven't seen it

@johnbieren If I understand correctly, the new fields in the advisory are not the intention here. They are only a side effect of adding additional data for the update-component-sbom task. I will refactor this PR to avoid that issue.

Thank you!

@johnbieren @mmalina I implemented your requested changes.

Note that stage Pyxis is down ATM, so e2e will fail right now.

@mmalina Pyxis is up, could you please run the tests?

/ok-to-test

Let's see if it is enough to start the tests.

E2E passed and this is approved. It is going to be competing with some other PRs for the rebase thing before merging, but now that we can rebase via UI, I will handle rebasing and merging this today. @wcheang this is ready to be merged, right?

@johnbieren wcheang is on holiday, I took over this PR. It is ready to be merged.

Just a heads-up, rebasing via the UI will remove signatures from commits.

@johnbieren wcheang is on holiday, I took over this PR. It is ready to be merged.

Just a heads-up, rebasing via the UI will remove signatures from commits.

thanks for the heads up. Yeah, we removed the signed commit requirement because having our contributors rebase 90x with us unable to help was not good 🙂 so now we can rebase for you

New changes are detected. LGTM label has been removed.

/ok-to-test

/ok-to-test

/ok-to-test